Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(editor): add support for trailing whitespace rendering #7215

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

alevinval
Copy link
Contributor

@alevinval alevinval commented Jun 3, 2023

Context

Adds a new render configuration value Trailing, which can be used to selectively enable trailing whitespace of certain whitespace characters as requested in #2719

@alevinval alevinval force-pushed the issue-2719 branch 3 times, most recently from 4533212 to a4c0fff Compare June 3, 2023 13:37
@alevinval alevinval changed the title feat(editor) highlight trailing whitespace WIP: feat(editor) highlight trailing whitespace Jun 4, 2023
@alevinval alevinval force-pushed the issue-2719 branch 3 times, most recently from 6d48eea to 4ee444a Compare June 4, 2023 19:52
Adds a new render configuration value `Trailing`, which can be used
to selectively enable trailing whitespace of certain whitespace characters.
@alevinval alevinval changed the title WIP: feat(editor) highlight trailing whitespace feat(editor): add support for trailing whitespace rendering Jun 4, 2023
@kirawi kirawi added the A-helix-term Area: Helix term improvements label Jun 5, 2023
self.viewport.x + offset as u16,
self.viewport.y + position.row as u16,
&trailing_whitespace[char_to_byte_idx(&trailing_whitespace, begin_at)..],
style,
Copy link
Member

@pascalkuthe pascalkuthe Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will messup diagnostic and selection. The style can frequently differ, you should only be updating the text while leaving cell style unchanged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I render something without being forced to provide a style?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure how to apply this feedback. I've changed the code to always apply whitespace_style though. I think this is basically the last pending thing to address.

@pascalkuthe pascalkuthe added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 21, 2023
@kirawi kirawi added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2023
@m0ar
Copy link

m0ar commented Oct 3, 2023

@pascalkuthe Would it be possible to the next round of review on this? 🙏 It's a feature I have been running a fork for and it would be great to see it included in the main editor, I'm sure @alevinval feels similarly :)

@kmicklas
Copy link
Contributor

When testing this I noticed that with many themes the trailing whitespace indicators are not particularly visible, which makes sense since they share a style with the "all" whitespace render feature. However, it's counterproductive if the goal is to have a very clear visible indicator of trailing whitespace. Is it worth considering a separate style class for this?

@alevinval alevinval force-pushed the issue-2719 branch 4 times, most recently from 5d30aa2 to ce08a77 Compare April 13, 2024 21:59
Helix is handling narrow non-breaking spaces, update
trailing whitespace tracker to take them into account
@alevinval
Copy link
Contributor Author

Hi @kmicklas, any pointers on where I should look in the codebase to land a custom style? So far I've pushed to keep this PR up-to-date with the latest master changes, which now also deal with narrow non-breaking spaces.

@alevinval
Copy link
Contributor Author

@kmicklas I realize now that adding the style appears to be quite trivial. I've just pushed 970122f, and with the following update on the config of my current theme:

diff --git a/runtime/themes/onelight.toml b/runtime/themes/onelight.toml
index e395716d..2f089060 100644
--- a/runtime/themes/onelight.toml
+++ b/runtime/themes/onelight.toml
@@ -138,6 +138,7 @@
 "ui.virtual.ruler" = { bg = "grey-200" }
 "ui.virtual.wrap" = { fg = "grey-500" }
 "ui.virtual.whitespace" = { fg = "grey-400" }
+"ui.virtual.trailing_whitespace" = { fg = "grey-300", bg = "red" }
 "ui.virtual.indent-guide" = { fg = "grey-500" }
 "ui.virtual.inlay-hint" = { fg = "grey-500" }
 "ui.virtual.inlay-hint.parameter" = { fg = "grey-500", modifiers = ["italic"] }

Results in the following:
demo-trailing-style

I presume I don't have to update any theme here, and that theme owners can choose how to best style it.

@gamma-delta
Copy link
Contributor

Hi, just putting here that I'd love this feature -- I write a lot of code for my job that I can't run a formatter on (because we don't have consistent formatting, or because it's some in-house language), and having this display immediately without needing to scour through the whole thing with all the whitespace symbols on or using git diff would be great.

@inducer
Copy link

inducer commented Jul 9, 2024

I've been using this, and I greatly appreciate the feature. I did notice an issue (though I am not meaning to hold up a potential merge):

If a line is truncated at the edge of the editor window, any spaces that happen to be at the end are flagged as trailing. This disappears once you scroll to the right.

Before scrolling:
grafik

After scrolling:
grafik

@daedroza
Copy link
Contributor

Can this feature be rebased on current HEAD instead of using merge tag?

@ElYaiko
Copy link

ElYaiko commented Sep 6, 2024

This works good, will this be merged on main?

@alevinval
Copy link
Contributor Author

Long time no code, I've updated this branch to latest master changes.

@kanashimia
Copy link

  1. Typing while having this option enabled is very distracting, especially if you set the highlighter to red background, your editor will blink every time you enter next word, as trailing whitespace is shown immediately. It works ok like that for regular whitespace.render = "all", but here it just isn't good, there needs to be some delay at least. You can also not render it on the same line as the primary cursor, but that will make it slightly confusing when you try delete the whitespace.

  2. Whitespace marker isn't rendered where it should be with tabs. Easier to see on the picture.

    whitespace.render="all":

    image

    whitespace.render="trailing":

    image

    Gray rectangle is the cursor.

  3. Trailing whitespace isn't rendered on the line with no newline.

@daedroza
Copy link
Contributor

Long time no code, I've updated this branch to latest master changes.

Hey, inside trailing_whitespace.rs

        WhitespaceKind::Tab => {
            let grapheme_tab_width = char_to_byte_idx(&palette.tab, 1);
            &palette.tab[..grapheme_tab_width]
        }

For grapheme_tab_width should be char_to_byte_idx(&palette.tab, palette.tab.len()). The default 1 value renders incorrectly when the character for tab is also space.

Indeed there are some other bugs but I'm happy with current changes.

@ntzm
Copy link

ntzm commented Nov 7, 2024

Works well for me, can you also add the new value into the docs https://github.com/helix-editor/helix/blob/master/book/src/editor.md#editorwhitespace-section

@nik-rev
Copy link
Contributor

nik-rev commented Nov 15, 2024

If there is some whitespace and your cursor is on the whitespace, it will become fully highlighted

image

So naturally I would expect that if I pressed d it would delete the entire highlighted region, but this isn't the case as just one newline is deleted, the whitespace isn't.

So I think it makes sense if the entire whitespace didn't get highlighted, OR the highlighted region was treated as a selection and would delete the whitespace

@senekor
Copy link
Contributor

senekor commented Feb 24, 2025

Hi, just putting here that I'd love this feature -- I write a lot of code for my job that I can't run a formatter on (because we don't have consistent formatting, or because it's some in-house language), and having this display immediately without needing to scour through the whole thing with all the whitespace symbols on or using git diff would be great.

@gamma-delta For your use case, it might be even better to configure a custom formatter that only trims trailing whitespace. Less annoying visually, less manual work. A sed command can do this, alternatively I made ec2hx which has that built-in. I also commented about this here and here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.